Find/Replace overlay: fix Ctrl+F6 crash on Linux #1975

Wittmaxi · 2024-06-17T11:24:23Z

Avoid being stuck in an event loop by reacting asynchronously to part activation events.
Additionally, manually give focus to the editor part after switching. This behavior is default on windows but not consistent on Linux.

how to test

after pressing Ctrl+F in a few different Editors,
(on Ubuntu), press Ctrl+6 repeatedly to switch between Editor parts. We (quite obviously) don't expect anything to crash!

fixes #1951

Wittmaxi · 2024-06-17T11:24:37Z

@iloveeclipse can you please confirm that this fixes your issue on Linux?

iloveeclipse

This PR fixes the problem on Linux, thanks. Some smaller issues see in comments above.

iloveeclipse · 2024-06-17T12:06:31Z

...es/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceOverlay.java

 			}
+
+			getShell().setVisible(isPartCurrentlyDisplayedInPartSash());


The code called from asyncExec() without checking whether preconditions are still met : the targetPart is still opened and it's shell is non null.

Same for the code below in the asyncExec() call.

I can add checks for whether the shell is still open, sure. How do I check whether the targetPart is still open? I didn't find a method for that

targetPart.getSite().getPart() would be null.

...es/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceOverlay.java

github-actions · 2024-06-17T12:08:27Z

Test Results

1 210 files ±0 1 210 suites ±0 1h 3m 33s ⏱️ + 3m 14s
7 663 tests ±0 7 431 ✅ - 1 231 💤 ±0 1 ❌ +1
16 100 runs ±0 15 586 ✅ - 1 513 💤 ±0 1 ❌ +1

For more details on these failures, see this check.

Results for commit 4289711. ± Comparison against base commit d67d6c7.

♻️ This comment has been updated with latest results.

Wittmaxi · 2024-06-17T13:50:56Z

I am not sure if my PR is failing but it looks like it isn't?

@fedejeanne

iloveeclipse · 2024-06-17T14:11:18Z

...es/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceOverlay.java

 		}

 		@Override
 		public void partDeactivated(IWorkbenchPart part) {
-			// Do nothing
+			Display.getDefault().asyncExec(this::adaptToPartActivationChange);


better to use getShell().getDisplay().asyncExec - here and below

ad63e43

NOTE: getShell() is not the "Eclipse Shell" but rather the shell of the Dialog.
Since this asynchronous execution is very quick, it's likely not a problem (?)

iloveeclipse · 2024-06-17T14:12:56Z

...es/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceOverlay.java

@@ -262,6 +258,32 @@ public void partClosed(IWorkbenchPart part) {
 		public void partOpened(IWorkbenchPart part) {
 			// Do nothing
 		}
+
+		private void adaptToPartActivationChange() {
+			if (getShell() == null || targetPart.getSite().getShell() == null) {


targetPart.getSite().getShell() == null that is same as getShell(). Instead, check for the targetPart.getSite().getPart() == null

No, that is not the same as getShell() as this code is ran from inside of a Dialog. getShell() returns the shell of the Dialog, and targetPart.getSite().getShell() returns the the shell which contains the part we are targetting.

Sure, I can change the check for whether the part is null. Please refer to my comment down-below.

Pleas ehcekc whether my change in ad63e43 is addressing your concerns

...es/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceOverlay.java

iloveeclipse · 2024-06-17T14:15:30Z

...es/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceOverlay.java

+		}
+
+		private void focusTargetWidget() {
+			if (targetPart.getSite().getShell() == null) {


targetPart.getSite().getShell() == null should be replaced with getShell() == null || targetPart.getSite().getPart() == null

I don't quite understand why that makes sense, but if it is true that targetPart.getSite().getPart() != null implies targetPart.getSite().getShell() != null, then that should work by contraposition.

Please keep in mind that this code is ran from inside a Dialog, so getShell() does not return the same shell as the shell the part is located in.

I performed the changes in ad63e43

fedejeanne · 2024-06-18T07:52:48Z

I am not sure if my PR is failing but it looks like it isn't?

@fedejeanne

LGTM now :-)

Once Andrey's remarks have been addressed, this PR can be merged-

HeikoKlare · 2024-06-27T15:38:56Z

@iloveeclipse have your concerns been addressed (in particular #1975 (comment)), so that we may merge this PR?

iloveeclipse · 2024-06-27T15:41:08Z

I haven't checked the PR since, but please rebase before merge, so we see test results with latest master state.

HeikoKlare · 2024-06-27T15:58:09Z

but please rebase before merge, so we see test results with latest master state.

Sure, I have just rebased it to get some recent test results but depending on when this PR will be marked as ready to be merged, we will rebase again.

Wittmaxi · 2024-07-01T06:22:52Z

@HeikoKlare @akurtakov I have merged this PR with the latest master

iloveeclipse

LGTM.

iloveeclipse · 2024-07-02T07:30:01Z

@Wittmaxi : thanks, I've finally had time to test the patch, it fixes the problem. Please manually rebase on master, as due the package move the change can't be merged.

Wittmaxi · 2024-07-02T09:37:18Z

@HeikoKlare I won't get to merge these changes until next monday. If you have some spare time and if it's straight-forward to do, could you please merge them to master?

HeikoKlare · 2024-07-03T16:09:54Z

I've rebased the changes on master. The local Git client is able to automatically consider the file relocation when performing a rebase, so rebase was performed without manual effort and thus without the risk of unintended changes/mistakes. Once the GitHub checks have finished successfully, we can merge this.

Edit: I had to apply a slight change concerning the way the source viewer is accessed (https://github.com/eclipse-platform/eclipse.platform.ui/compare/23d62ec2c50dc399b7049afdebe22922b7dcf70d..6e143ebff2b1705d52e9f4e1984e7faadcf71175), exactly like applied at other places when moving the find/replace classes in a separate package (#2013).

Avoid being stuck in an event loop by reacting asynchronously to part activation events. Additionally, manually give focus to the editor part after switching. This behavior is default on windows but not consistent on Linux. fixes eclipse-platform#1951

HeikoKlare · 2024-07-04T08:13:39Z

I've tested the change on Linux, Windows and MacOS. I can also confirm that it fixes the reported bug on Linux and also works as expected on Windows and MacOS.

Wittmaxi mentioned this pull request Jun 17, 2024

Find/Replace overlay: handle the case when moving editor between windows #1946

Merged

iloveeclipse requested changes Jun 17, 2024

View reviewed changes

Wittmaxi force-pushed the task_4-find/replace_overlay-fi branch 2 times, most recently from b85fd61 to a5a9122 Compare June 17, 2024 12:46

iloveeclipse reviewed Jun 17, 2024

View reviewed changes

...es/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceOverlay.java Outdated Show resolved Hide resolved

iloveeclipse reviewed Jun 17, 2024

View reviewed changes

Wittmaxi force-pushed the task_4-find/replace_overlay-fi branch 2 times, most recently from 1f97f91 to ad63e43 Compare June 22, 2024 10:14

Wittmaxi requested a review from iloveeclipse June 24, 2024 06:58

HeikoKlare force-pushed the task_4-find/replace_overlay-fi branch from ad63e43 to 09865c8 Compare June 27, 2024 15:58

Wittmaxi force-pushed the task_4-find/replace_overlay-fi branch from 09865c8 to 126ba0d Compare July 1, 2024 06:22

iloveeclipse approved these changes Jul 2, 2024

View reviewed changes

HeikoKlare force-pushed the task_4-find/replace_overlay-fi branch from 126ba0d to 23d62ec Compare July 3, 2024 16:06

HeikoKlare force-pushed the task_4-find/replace_overlay-fi branch from 23d62ec to 6e143eb Compare July 3, 2024 16:20

HeikoKlare force-pushed the task_4-find/replace_overlay-fi branch from 6e143eb to 4289711 Compare July 3, 2024 17:09

HeikoKlare merged commit f8acd2d into eclipse-platform:master Jul 4, 2024
15 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Find/Replace overlay: fix Ctrl+F6 crash on Linux #1975

Find/Replace overlay: fix Ctrl+F6 crash on Linux #1975

Wittmaxi commented Jun 17, 2024

Wittmaxi commented Jun 17, 2024

iloveeclipse left a comment

iloveeclipse Jun 17, 2024

Wittmaxi Jun 17, 2024

iloveeclipse Jun 17, 2024

github-actions bot commented Jun 17, 2024 •

edited

Loading

Wittmaxi commented Jun 17, 2024

iloveeclipse Jun 17, 2024

Wittmaxi Jun 22, 2024

iloveeclipse Jun 17, 2024

Wittmaxi Jun 22, 2024

Wittmaxi Jun 22, 2024

iloveeclipse Jun 17, 2024

Wittmaxi Jun 22, 2024 •

edited

Loading

Wittmaxi Jun 22, 2024

fedejeanne commented Jun 18, 2024

HeikoKlare commented Jun 27, 2024

iloveeclipse commented Jun 27, 2024

HeikoKlare commented Jun 27, 2024

Wittmaxi commented Jul 1, 2024

iloveeclipse left a comment

iloveeclipse commented Jul 2, 2024

Wittmaxi commented Jul 2, 2024

HeikoKlare commented Jul 3, 2024 •

edited

Loading

HeikoKlare commented Jul 4, 2024

		}

		getShell().setVisible(isPartCurrentlyDisplayedInPartSash());

Find/Replace overlay: fix Ctrl+F6 crash on Linux #1975

Find/Replace overlay: fix Ctrl+F6 crash on Linux #1975

Conversation

Wittmaxi commented Jun 17, 2024

how to test

Wittmaxi commented Jun 17, 2024

iloveeclipse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 17, 2024 • edited Loading

Test Results

Wittmaxi commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wittmaxi Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedejeanne commented Jun 18, 2024

HeikoKlare commented Jun 27, 2024

iloveeclipse commented Jun 27, 2024

HeikoKlare commented Jun 27, 2024

Wittmaxi commented Jul 1, 2024

iloveeclipse left a comment

Choose a reason for hiding this comment

iloveeclipse commented Jul 2, 2024

Wittmaxi commented Jul 2, 2024

HeikoKlare commented Jul 3, 2024 • edited Loading

HeikoKlare commented Jul 4, 2024

github-actions bot commented Jun 17, 2024 •

edited

Loading

Wittmaxi Jun 22, 2024 •

edited

Loading

HeikoKlare commented Jul 3, 2024 •

edited

Loading